Skip to content

Comments

_varargs_labels_as_list test#597

Closed
tiddoloos wants to merge 1 commit intodata-8:masterfrom
tiddoloos:master
Closed

_varargs_labels_as_list test#597
tiddoloos wants to merge 1 commit intodata-8:masterfrom
tiddoloos:master

Conversation

@tiddoloos
Copy link

[ ] Wrote test for _varargs_labels_as_list

Changes proposed:
tests for _varargs_labels_as_list

@coveralls
Copy link

Coverage Status

coverage: 93.535% (+0.08%) from 93.455% when pulling a2c0ca3 on tiddoloos:master into ec3ebcd on data-8:master.

Copy link
Member

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most languages, testing "private" or "internal" functions is not a pattern that is encouraged (if that pattern even allows it at all). While I appreciate the additional test coverage these tests provide greatly, I think it would be best if we made test cases using Table#drop and Table#select to trigger all the conditional statements within Table#_varargs_labels_as_list instead. Let me know your thoughts.

@adnanhemani
Copy link
Member

Closing PR as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants